add terraform to provision packet machine by paulfantom · Pull Request #22 · openshift/cluster-api-provider-libvirt

paulfantom · 2018-09-21T15:31:57Z

Add a script to provision packet machine and configure it so we can connect to it with libvirt client via SSH or TCP. It also automatically creates temporary ssh key file if needed (default behavior).

/wip

paulfantom · 2018-09-21T15:34:40Z

/hold

paulfantom · 2018-09-21T22:05:21Z

/hold cancel

enxebre · 2018-09-27T14:26:13Z

overall looks great to me. Not a blocker at all but we'll probably want parameterise the ip range for the iptable rule

spangenberg · 2018-09-27T14:31:59Z

hack/prebuild/main.tf

+}
+
+resource "packet_ssh_key" "key" {
+  name       = "unlikely_tf_ssh_key_name"


You should put ${var.environment_id} here.

spangenberg · 2018-09-27T14:32:05Z

hack/prebuild/main.tf

@@ -0,0 +1,32 @@
+resource "packet_project" "libvirt_actuator" {
+  name = "libvirt-actuator tests"


Maybe put ${var.environment_id} here.

spangenberg · 2018-09-27T14:32:54Z

hack/prebuild/main.tf

+}
+
+resource "packet_device" "libvirt" {
+  hostname         = "${var.environment_id}"


Prepend something non alpha numeric, in case uuid generates a number fist.

bison · 2018-09-27T15:17:17Z

hack/prebuild/init.sh

+
+echo 'LIBVIRTD_ARGS="--listen"' >> /etc/sysconfig/libvirtd
+
+iptables -I INPUT -p tcp --dport 16509 -j ACCEPT -m comment --comment "Allow insecure libvirt clients"


Are the tests going to use TCP or SSH as a transport for libvirt? I don't think it's okay to run these with unauthenticated libvirt listening on the public internet.

For CI I was thinking on parameterising the ip range allowed to access and then reuse your tls stuff, so we test it as it's consumed by the installer/mao. wdyt?

Yeah, that sounds good. I'm not familiar with the e2e tests or the plan for those. Do they use the installer? If so, I think this should be on-hold until openshift/installer#296 merges.

Eventually we might use installer. Currently for mao/aws-actuator we use terraform to create the minimal infra needed to satisfy some actuator assumptions and verify expected behaviour,
See https://github.com/openshift/machine-api-operator/tree/master/tests
e.g The minimal infra for libvirt actuator will contain a network and the ign volumes

Ah, cool. Then I think it should be pretty easy to use TLS or SSH as the transport from the start. There aren't really any changes required for the actuator. The credentials, certs or SSH keys, just need to be available and the right URI needs to be used. The libvirt library will handle the rest.

Just to add a little more detail on making TLS work: here's a link to the docs

We just need to generate the TLS assets somehow -- could use certtool, cfssl, or even Terraform if we don't want to use the Go tool in the installer PR. Then configure libvirtd to point to the certs and use a URI like: qemu+tls://$IP/system?pkipath=/path/to/certs

That should be enough and pretty secure as long as no one can access the generated certs and they are one-time use.

It would be probably easier to adapt it to use SSH as this already need SSH keys to connect to instance. WDYT?

Totally fine. As long as we're doing authentication and encryption somehow.

bison · 2018-09-27T15:19:49Z

hack/prebuild/main.tf

@@ -0,0 +1,32 @@
+resource "packet_project" "libvirt_actuator" {


Does this create a project for every build? We probably want a single re-used project. That will let us manage permissions for it.

spangenberg · 2018-09-27T15:23:53Z

/hold

paulfantom · 2018-09-28T13:31:30Z

/hold cancel

frobware · 2018-09-28T14:23:50Z

hack/packet-provision.sh

+set +e
+
+# Your Packet user account
+if [ "$PACKET_AUTH_TOKEN" == "" ]; then


-z vis-a-vis == ""?

spangenberg · 2018-09-28T14:28:39Z

/lgtm

openshift-ci-robot · 2018-09-28T14:28:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paulfantom, spangenberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [paulfantom,spangenberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

frobware · 2018-09-28T14:25:38Z

hack/packet-provision.sh

+        ssh_path="/tmp/packet_id_rsa.pub"
+    fi
+    terraform init -input=false
+    terraform plan -input=false -out=tfplan.out && terraform apply -input=false -auto-approve tfplan.out


As we have -e this could be two separate commands.

Right, I will fix that in a follow-up PR

frobware · 2018-09-28T14:26:26Z

hack/packet-provision.sh

+    ;;
+  "destroy")
+    terraform destroy -input=false -auto-approve
+    rm /tmp/packet_id_rsa* || :


Why not use rm -f, instead of ||?

frobware · 2018-09-28T14:29:26Z

hack/prebuild/init.sh

+#/bin/bash
+
+yum install -y -d1 libvirt libvirt-daemon-kvm
+usermod -aG libvirt root


Not sure I've looked to see how this script is used (or when) but do you not need newgrp libvirt here having modified the groups?

It is used to install libvirt on packet host the easiest way.

frobware · 2018-09-28T14:31:18Z

hack/prebuild/init.sh

+
+iptables -I INPUT -p tcp --dport 16509 -j ACCEPT -m comment --comment "Allow insecure libvirt clients"
+
+systemctl start libvirtd


Do we also need systemctl enable libvirtd? Is there any expectation of this starting on reboot?

No, those machines aren't expected to be rebooted.

Incorporate suggestions from #22

add terraform to provision packet machine

f3d7658

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 21, 2018

openshift-ci-robot requested review from trawler and vikaschoudhary16 September 21, 2018 15:32

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2018

paulfantom changed the title ~~add terraform to provision packet machine~~ [WIP] add terraform to provision packet machine Sep 21, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2018

paulfantom changed the title ~~[WIP] add terraform to provision packet machine~~ add terraform to provision packet machine Sep 27, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2018

spangenberg suggested changes Sep 27, 2018

View reviewed changes

bison reviewed Sep 27, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2018

remove project creation; better id; disable tcp+qemu access

98e24f9

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2018

fix go fmt execution in Makefile

e50a89d

frobware reviewed Sep 28, 2018

View reviewed changes

hack/packet-provision.sh

set +e

# Your Packet user account

if [ "$PACKET_AUTH_TOKEN" == "" ]; then

Copy link

Contributor

frobware Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-z vis-a-vis == ""?

openshift-ci-robot assigned spangenberg Sep 28, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2018

spangenberg approved these changes Sep 28, 2018

View reviewed changes

openshift-merge-robot merged commit 83c10e8 into master Sep 28, 2018

frobware reviewed Sep 28, 2018

View reviewed changes

paulfantom deleted the packet branch September 28, 2018 14:38

openshift-merge-robot added a commit that referenced this pull request Oct 1, 2018

Merge pull request #30 from paulfantom/fix_22

7284131

Incorporate suggestions from #22

		@@ -0,0 +1,32 @@
		resource "packet_project" "libvirt_actuator" {
		name = "libvirt-actuator tests"


		echo 'LIBVIRTD_ARGS="--listen"' >> /etc/sysconfig/libvirtd

		iptables -I INPUT -p tcp --dport 16509 -j ACCEPT -m comment --comment "Allow insecure libvirt clients"


		iptables -I INPUT -p tcp --dport 16509 -j ACCEPT -m comment --comment "Allow insecure libvirt clients"

		systemctl start libvirtd

Conversation

paulfantom commented Sep 21, 2018

Uh oh!

paulfantom commented Sep 21, 2018

Uh oh!

paulfantom commented Sep 21, 2018

Uh oh!

enxebre commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulfantom Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bison Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spangenberg commented Sep 27, 2018

Uh oh!

paulfantom commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spangenberg commented Sep 28, 2018

Uh oh!

openshift-ci-robot commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

enxebre Sep 27, 2018 •

edited

Loading

paulfantom Sep 27, 2018 •

edited

Loading

bison Sep 27, 2018 •

edited

Loading